Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

packages/tablicious.yaml: release 0.3.7, 0.4.0, 0.4.1, 0.4.2 #401

Merged
merged 2 commits into from
Feb 9, 2024

Conversation

mmuetzel
Copy link
Member

@mmuetzel mmuetzel commented Feb 8, 2024

@apjanke: Does that look correct to you?

@mmuetzel
Copy link
Member Author

mmuetzel commented Feb 8, 2024

One of the doctests is failing:

      message = parse error near line 48 of file /usr/share/octave/packages/tablicious-0.4.2/+tblish/+internal/mycombvec.m
    syntax error
  >>>         out = [out; [repmat (a(i), [size (rest_combs,1) 1]) rest_combs]];
                                        ^
      identifier = 
      stack =
        6x1 struct array containing the fields:
          file
          name
          line
          column
      done.

Spaces separate elements inside brackets [ ]. So, that line is interpreted as out = [out; [repmat, (a(i), [size, (rest_combs,1), 1]), rest_combs]];. Combining comma-separated lists with parenthesis is not supported (and is probably not what you wanted).
That line should probably be the following instead:

out = [out; [repmat(a(i), [size(rest_combs,1), 1]), rest_combs]];

(I added some commas for clarity that aren't actually needed. But they make it a bit easier to read imho.)

@mmuetzel
Copy link
Member Author

mmuetzel commented Feb 8, 2024

Additionally, some pkg-tests are failing:

  >>>>> processing /usr/share/octave/packages/tablicious-0.4.2/@datetime/datetime.m
  ***** test datetime ('2011-03-07 12:34:56', 'TimeZone', 'America/New_York');
  !!!!! test failed
  Undefined TimeZone: America/New_York
  ***** test
    d = datetime;
    d.TimeZone = 'America/New_York';
    d2 = d;
    d2.TimeZone = 'America/Chicago';
    assert (abs (datenum (d) - datenum (d2)), (1/24), .0001)
  !!!!! test failed
  Undefined TimeZone: America/New_York

Not sure what that means.

@apjanke
Copy link
Contributor

apjanke commented Feb 9, 2024

The new versions

Does that look correct to you?

Yep, those look correct to me. I didn't verify the SHA sums, but all the other info looks right.

Assuming those dates are UTC, that is. Like, they're supposed to be the calendar day of the UTC time of the release, and not my nominal local time. Historically, I put the local calendar day in my CHANGES.txt and documentation, and I'm in the US, so for evening releases it won't line up. E.g. in my doco and notes, I have v 0.3.7 with release date 2023-01-05, not 2023-01-06.

Your UTC date approach is better. I think I'll change my notes and doco to match that. Ticket for that: apjanke/octave-tablicious#126.

Reproducing the failure

That's odd: I'm not getting that failure in tblish.internal.mycombvec when I run the tests myself, under Octave 8.4.0 on macOS, like follows. Do I not understand what a "doctest" is?

>> __run_test_suite__ ({'/Users/janke/repos/octave-tablicious/inst'}, {})

Integrated test scripts:

  /Users/janke/repos/octave-tablicious/inst/@datetime/datetime.m . pass    4/4
  /Users/janke/repos/octave-tablicious/inst/@duration/duration.m . pass    3/3
  /Users/janke/repos/octave-tablicious/inst/@string/string.m ..... pass    6/6
  /Users/janke/repos/octave-tablicious/inst/NaS.m ................ pass    1/1
  /Users/janke/repos/octave-tablicious/inst/missing.m ............ pass    2/2

Fixed test scripts:


Summary:

  PASS                               16
  FAIL                                0

The [... foo (...) ...] syntax thing

Anyway, yep, you're totally right. Those extra spaces were a mistake. Probably introduced recently when I did this "Convert to GNU Octave code style" change, where I added a space at function calls to be "foo (...)" instead of "foo()", but didn't catch by manual inspection the places where that wasn't correct.

This affected a few other functions, too. And in some cases, they were single-arg function calls, so no commas in the "(...)", which I think means they weren't syntax errors, but were incorrectly calling the function with zero args, and then concatenating its return value with the "(...)" expression that was supposed to be the function args, maybe raising a run-time error or just producing incorrect results. That's worse.

I've checked in a hopeful fix in apjanke/octave-tablicious@151d2ec and will be merging it soon and rolling a corrective patch release. Hopefully get some more unit test coverage in there too.

Thanks for the catch!

The invalid time zone thing

The datetime test looks like a problem locating the tzdb data on this computer. Let's handle that under the Tablicious bug you opened here: apjanke/octave-tablicious#125.

@mmuetzel
Copy link
Member Author

mmuetzel commented Feb 9, 2024

Assuming those dates are UTC, that is. Like, they're supposed to be the calendar day of the UTC time of the release, and not my nominal local time.

Oops. I didn't even consider to look at the time. I just took the dates that GitHub presented to me (current timezone UTC+1). If you prefer dates in your time zone, that would also be fine.

Do I not understand what a "doctest" is?

Afaict, the doctests are run using the doctest package. I.e., something like following would probably run them locally:

pkg install -forge doctest  # only needed once per Octave version
pkg load doctest
doctest('path/to/your/m_files')

@mmuetzel mmuetzel force-pushed the tablicious branch 3 times, most recently from 5e589cd to 1915dba Compare February 9, 2024 08:31
@mmuetzel
Copy link
Member Author

mmuetzel commented Feb 9, 2024

The datetime test no longer fails after installing the Ubuntu package tzdata.

@siko1056: I had to adapt octave_ci.m slightly to avoid interactive prompts in the CI (that would block it indefinitely because there won't be any input).
Do those changes look good to you?

@mmuetzel
Copy link
Member Author

mmuetzel commented Feb 9, 2024

The issue with the CI script doesn't need to hold back the update of the tablicious package.

I stripped those commits and will open a new PR for those changes.

Merging the part that updates the list of releases of the tablicious package.

@mmuetzel mmuetzel merged commit 5681cb2 into gnu-octave:main Feb 9, 2024
2 of 4 checks passed
@apjanke
Copy link
Contributor

apjanke commented Feb 9, 2024

If you prefer dates in your time zone, that would also be fine.

I prefer UTC, actually. I just wasn't really thinking about it when I wrote down those older dates.

Yep, that pkg install -forge doctest worked for me. I'm seeing some failures in those tests. I'll check them out. Handling that here: apjanke/octave-tablicious#129

@apjanke
Copy link
Contributor

apjanke commented Feb 10, 2024

Say, @mmuetzel, when I make a new Tablicious release, should I do a PR here to add another entry like you did for these existing releases? Or just leave it up to y'all as a downstream packager?

I'm probably gonna roll another Tablicious release tonight or tomorrow; good time for a trial run if I should incorporate this in to my workflow.

@apjanke
Copy link
Contributor

apjanke commented Feb 10, 2024

Oh, and is there a way for me to publish release notes somewhere (like in the package repo itself, or on the GitHub site) so that they're included in the release description for the generated Discourse announcement posts like this one?

https://octave.discourse.group/t/tablicious-0-4-2-released/5270

@siko1056
Copy link
Member

siko1056 commented Feb 11, 2024

@apjanke the announcement in Discourse is a normal message, that you can edit as you please. For Octave packages I think that version release notes were mostly distributed via the news package_name command. Of course the repo readme or GitHub release page can be used as well.

If there is a need for another release notes space, we can discuss and extend Octave packages of course.

@siko1056 siko1056 changed the title tablicious: Add new versions packages/tablicious.yaml: release 0.3.7, 0.4.0, 0.4.1, 0.4.2 Feb 12, 2024
@siko1056 siko1056 added the package release New package release label Feb 12, 2024
@mmuetzel
Copy link
Member Author

Say, @mmuetzel, when I make a new Tablicious release, should I do a PR here to add another entry like you did for these existing releases? Or just leave it up to y'all as a downstream packager?

It would be really nice if you could update the .yaml as part of your release process. 👍
Please let us know if you need help with that. But adding a new version should be easy if you just follow the existing pattern.

Fwiw, pkg install -forge uses the information provided in this repository. So, keeping the files here up-to-date would facilitate the installation (and update) process of your package.

If you maintain more Octave packages that aren't listed yet, you are welcome to add them here, too.

@apjanke
Copy link
Contributor

apjanke commented Feb 13, 2024

It would be really nice if you could update the .yaml as part of your release process. 👍

Cool. Looks straightforward. Added a note to my Release Checklist doc here. Sound right to you?

If you maintain more Octave packages that aren't listed yet, you are welcome to add them here, too.

I've got a couple other Octave packages, but none of them are mature or active enough to be listed in the package index at this point.

the announcement in Discourse is a normal message, that you can edit as you please.

Oh, yup: I could edit it no problem. Thanks.

image

[...] version release notes were mostly distributed via the news package_name command.

Oh. Looks like I need to update Tablicious to support news. Doing in apjanke/octave-tablicious#132.

Thanks, y'all!

@apjanke
Copy link
Contributor

apjanke commented Feb 13, 2024

Is it okay if I update the Tablicious.yaml here to use my [email protected] email address, and point the "fa-book" documentation link at my GitHub Pages online documentation site at https://apjanke.github.io/octave-tablicious/ instead of the README in the repo? That's where I'd like users to head first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package release New package release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants